Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

can @kassens @yuzhi @lukecwilliams @josephsavona have a look?

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Hi @josephsavona
thanks for reviewing!
can you put some comments on my PR statements (i.e. fragment and containers stuff...)
the challenge I can see is that, even we implement a lazy api that returns _getFragmentVariables() result, that won't return you the correct variables. because getFragmentVariables will always return the default value because fragment are statically built and are agnostic about containers
think about having 2 components rendered on the same screen. How can we find the correct variables?

In addition to what Joe mentioned, I recommend avoiding overloading localVariables with this use case. It contains variables used for the refetch query, which is not always the same set of variables used to read data out. It may contain more variables and variables that might be different from what's used to read.

@yuzhi you are absolutely right. but isn't the updated variables that we all care about?
e.g. in your application, you first read data using { showComments: false } then you do a refetch with {showComments: true}.
now, refetch has completed and you component gets re-rendered, you need to make a decision on whether to show comment section or not (you only want to display comments section once data is ready), so you consult relay to get current variables.
say we have an lazy api called getVariables(), so what do you expect from getVariables() ? do you expect it to return false (fragment variables. used to read data. always false) or true which is refetch variables (live variables. per instance variables. container variables...)

hmm... I think we can do a data check to reverse engineering what variables we used to get them...

e.g.

if(this.props.story.comments){
   // data re-fetched with {$showComments: true}

}else{
   // data initially read out with fragment variables { $showComments: false}
 }

then we don't need variables at all

the problem I can see is, say, a user clicks the show comments button, but there is no comments associated with that story. so , by reverse engineering data, the application might think that this is the initial read, while in fact it's a refetch

hope it all makes sense...

getFragmentVariables will always return the default value because fragment are statically built and are agnostic about containers think about having 2 components rendered on the same screen. How can we find the correct variables?

I don't follow. getFragmentVariables() accounts for defaults, root variables, and fragment arguments (if fragment-local variables are being used). Of course it's possible that there's a bug - can you provide a failing case in https://github.com/facebook/relay/blob/master/packages/relay-runtime/store/__tests__/RelayConcreteVariables-test.js#L29 to help clarify the discussion?

@bochen2014 Yeah this type of reverse engineering is error-prone. I agree that we should have some way of getting the variables used to render the current data, when necessary.

ah.. we can check the data-structure , not the actually data;
as long as we can see the comments field, we always know that this is a re-fetch. even when the actually value is undefined or null or '' or []

@josephsavona spot on! I had the same question

1. check relay\packages\relay-runtime\store\RelayConcreteVariables.js, line 42

variables[definition.name] = definition.defaultValue; 

this will make sure that default value always get returned no matter what argumentVariables are

2. check relay\packages\relay-runtime\store\RelayModernSelector.js line 79

 const argumentVariables = fragments[fragment.name];
 const fragmentVariables = getFragmentVariables(
      fragment,
      operationVariables,
      argumentVariables,
    );

the argumentVariables we passed in is fragment. nothing to do with variables. I was wondering if this is intended

@josephsavona I think the unit test passed because you didn't specify a default value in argument definition

https://github.com/facebook/relay/blob/master/packages/relay-runtime/store/__tests__/RelayConcreteVariables-test.js#L39

@bochen2014 Nope, test passes with a default value too. This line ensures that an argument value is preferred over a default/root value. Is there another case that you are thinking of? You might be seeing an unrelated bug.

right ..... can you look at
https://github.com/facebook/relay/blob/master/packages/relay-runtime/store/RelayModernSelector.js#L79
and see if we have a problem there?

That's intended, fragment-local variables are passed via an object that keys by fragment name. Can you provide a complete example of the behavior you're seeing and how you'd expect it to behave?

I have seen what you said in classic relay and thought that has changed. but fortunately we are still keeping it. great!

does the screenshot sort of make sense to you?

The screenshot doesn't provide enough context to debug. Can you describe the sequence of events and what data you get vs what you expect? Are you using fragment-local variables - @argumentDefinitions and @arguments?

I expose _getFragmentVariables here

I also have this as an indicate that refetch has completed

here i am getting runtime variables

my this.props.relay.getVariables() always return default value

Ah ok. Simply exporting _getFragmentVariables() will only return the variables determined from props and the ancestor root query's variables. Adding a getVariables() function will require using _getFragmentVariables() unless a refetch has occurred, in which case the local variables should be used (and as @yuzhi noted, those are a subset of _localVariables).

I could be wrong. but it seems that my @arguments are never reflected in my this.props even after a refetch -- but @josephsavona has confirmed that it should be the case

That's intended, fragment-local variables are passed via an object that keys by fragment name

so it either be a bug, or I'm doing something wrong

I'm now fully convenced that exposing getVariables as a lazy api is a better solution comparing to exposing _localVariables. totally agree on that. look forward to seeing how it is implemented because it didn't work for me for the reason I mentioned above

I was expecting my @argments be part of viewer.__fragments.tolist_viewer after a refetch

but it's always empty -- that made me think that getFragmentVariables always return default value. but later on you pointed out that we have

 if (argumentVariables.hasOwnProperty(definition.name)) {
      return;
    }

which I missed...

Yeah this is tricky: in your screenshot this.props are the props received from the refetch container's parent, which isn't affected by the refetch. The refetch container internally updates the props it provides to the component, using the expected variables.

It's great that you're digging in and experimenting with the core functions! Care to try updating the getFragmentVariables() function with the logic I described?

yep! I will have a try ;-) thanks @josephsavona . that has been extremely informative and helpful!

Sure thing, thanks for contributing!

done. a piece of cake after you explained how it works (or supposed to work) . thanks @josephsavona again!

Can you add some unit tests?

yes I was expecting someone to ask ;-) haha

Epic thread. 😄

I addressed the superset/subset issue raised by @yuzhi in the updated PR; I have also added a new unit test and refactored existing unit test.

can I ask someone have a review, as we need this as part of our project migration (relay-classic to relay-modern)

1. the failed unit test is Cannot find module 'RelayModern' from 'commitRelayModernMutation-test.js' which @AGS- may have a better idea;
2. my previous change on refetchContainer-test was quite aggressive and hard to review.
   this time, I limit the changes to minimum required. So it should be very easy to reivew

@josephsavona @yuzhi can you guys have a look ? thanks!

@bochen2014 Let me see what's up with the test failing tomorrow morning

@bochen2014 it should be fixed now

thanks @AGS- . the unit tests now all passed..

can I ask someone to review my changes again and point out issues/problems if any?
our web site relies heavy on query variables and this PR is critical for our relay-moder migration. thanks!

this PR is getting really messy. will create a new one instead